SSL hostname verification support #262
Conversation
No tests yet, just an initial implementation to discuss.
|
I tried to write this so it's possible to pass I made the code |
| @@ -88,6 +88,13 @@ def self.wrap_with_ssl(io, tls_options = {}) | |||
| conn = OpenSSL::SSL::SSLSocket.new(io, ctx) | |||
| conn.connect | |||
|
|
|||
| if tls_options[:verify_host] | |||
| # This raises OpenSSL::SSL::SSLError if hostname verification fails | |||
| conn.post_connection_check(tls_options[:verify_host]) | |||
There was a problem hiding this comment.
A previously undocumented (until I documented it) method it is very important to call if you want your SSL connections to be secure!
|
So this is a second pull request to address this same issue #258. My initial hack was pull #259. This pull request looks neater and more funcitonal than #259 in terms of parsing the hostname info through the function calls and doing the validation according to the
Users could force old ruby behaviour with, but arguably they should just use plain LDAP / 389 instead then. |
| @@ -36,7 +36,7 @@ def prepare_socket(server) | |||
| encryption = server[:encryption] | |||
|
|
|||
| @conn = socket | |||
| setup_encryption encryption if encryption | |||
| setup_encryption({ verify_host: server[:connected_host] }.merge(encryption)) if encryption | |||
There was a problem hiding this comment.
Note that this does verification by default unless explicitly disabled
There was a problem hiding this comment.
@tarcieri, sorry I'm not really much of a ruby dev, so I may misunderstand a bit. I assume any non nil value in a conditional returns false... so users must explicitly enable verification in config with :encryption = { :method => :simple_tls, :tls_options => { :verify_host => true } }?
If I understand correctly, :encryption => { :method => :simple_tls, :tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_PEER, :tls_options[:verify_host] => false} } should imply checking that the cert is at least signed by a trusted CA, but allowing for a mismatch?
What happens if the user has simply put { :method => :simple_tls, :tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_PEER }?
if tls_options[:verify_host]
# This raises OpenSSL::SSL::SSLError if hostname verification fails
conn.post_connection_check(tls_options[:verify_host])
The match function will not run if tls_options[:verify_host] is nil? Hence I thought that your pull doesn't do hostname validation unless the user explicitly configures it.
There was a problem hiding this comment.
No, this is passing the hostname through as verify_host, unless the user has set verify_host to something else.
It's a bit convoluted, but it's on by default, and enables the user to opt-out.
I would agree having a separate user targeted flag would make sense, but you don't seem to be understanding how this code is working.
There was a problem hiding this comment.
@tarcieri, thanks, got it now. I cloned your fork and tested :verify_host => false (with valid variables set for my env). It bumps into an error.
Can you provide some config examples?
conn = Net::LDAP.new :host => hostname,
:port => 636,
:base => base,
:encryption => { :method => :simple_tls,
:tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_PEER,
:verify_host => false,
:ca_file => ca_file } },
:auth => { :username => login,
:password => pass,
:method => :simple }
conn.bind
/usr/lib/ruby/2.1.0/openssl/ssl.rb:87:in `block in set_params': undefined method `verify_host=' for #<OpenSSL::SSL::SSLContext:0x000000016f4130> (NoMethodError)
from /usr/lib/ruby/2.1.0/openssl/ssl.rb:87:in `each'
from /usr/lib/ruby/2.1.0/openssl/ssl.rb:87:in `set_params'
from /home/<username>/Ruby/LDAPS/ruby-net-ldap-fix2/lib/net/ldap/connection.rb:86:in `wrap_with_ssl'Also, testing my own pull, I noticed that the CI build will fail if it actually checks the hostname by default because the CI test suite has a cert mismatched resulting in
Error: test_bind_tls_with_cafile(TestBindIntegration): Net::LDAP::Error: hostname "localhost" does not match the server certificate
So that implies your pull doesn't match the hostname by default because the CI build succeeded. I had the same confusing issue with my attempted fix.
|
@JPvRiel your PR is fine. I'd say let's go with that |
|
@tarcieri I thought your approach was technically neater by passing the host address info through the function calls to Regardless, I also saw notes that the connection handling stuff might be refactored... |
The current
Net::LDAP::Connection::wrap_with_sslverifies the certification paths of SSL certificates correctly, but it does not verify hostnames. This means any service with a certificate issued by the same CA as the LDAP server can impersonate the LDAP server. (Note: this is probably CVE worthy. Let me know if you'd like help getting a CVE assigned)This PR is more of a strawman implementation than anything to get the ball rolling towards a solution. I think it could definitely use some tests to ensure hostname verification works! But to do that we might want to talk about how to issue all of the certificates needed in the tests.